-
Notifications
You must be signed in to change notification settings - Fork 219
Improve Products block Attributes Filter Inspector Controls #8583
Improve Products block Attributes Filter Inspector Controls #8583
Conversation
Removes feature flag from: * Product Summary * Product Template * Product Title
* Move it into the shared hooks. * Fetch both terms AND attributes via the API (previously, we got the attributes from the settings, but we'd get partial objects compared to the API? Maybe a follow-up to this could be to check why the attributes stored in the settings are incomplete?) * Make sure the return values match the ones expected from search items.
* Allow the search input to be a Token based input. * Allow for search input to search even collapsed properties. * Use core `CheckboxControl` instead of radio buttons for items having children (includes undeterminated state). Please enter the commit message for your changes. Lines starting
* Refactor classnames for `SearchItem`. * Add more semantic classes. * Align count label and caret to the right. * Make caret switch direction on expanded. * `cursor: pointer` on collapsible items. * Indent children of collapsible items.
* If indeterminate or unchecked, it will check all children. * If checked, it will uncheck all children.
The release ZIP for this PR is accessible via:
Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/blocks/active-filters/block.tsx
assets/js/blocks/attribute-filter/block.tsx assets/js/blocks/product-query/inspector-controls/attributes-filter.tsx assets/js/blocks/products-by-attribute/edit-mode.tsx assets/js/blocks/products-by-attribute/inspector-controls.tsx assets/js/editor-components/product-attribute-term-control/index.tsx assets/js/editor-components/search-list-control/item.tsx |
Size Change: +15.6 kB (+1%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
@nefeline This should now be ready for review 🙏 |
Awesome! I'm getting started now 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! It's fantastic to see all of these improvements in place 🎉
I have encountered some errors, all of them detailed below:
Test results:
Simple happy path
Add a Products (Beta) block to your page.
Open the Inspector Controls and add the “Product Attributes” advanced filter.
The Attributes selector component should show up in an uninteractable loading state.
✅ Confirmed the attributes selector component shows up as expected.
Once loading finishes, the Attributes should appear, collapsed.
✅ Confirmed the attributes show up collapsed:
Uncollapse a section
Select a few attributes.
✅ Confirmed it's possible to uncollapse the sections and select the terms:
Publish your page.
Check on the frontend to see that the correct products are displayed.
❌ At this stage, on the FE, a PHP notice is triggered, and the page is blank:
Notice: Undefined index: taxonomy in /Users/patriciahillebrandt/Woo/plugins/woocommerce-blocks/src/BlockTypes/ProductQuery.php on line 301
I was able to circumvent this error by doing a quick refactor to the get_product_attributes_query
method as the $attributes
variable received has a different format than the one expected (e.g. it doesn't include the 'taxonomy' name for example.):
This is the quick fix I put in place to circumvent it:
private function get_product_attributes_query( $attributes = array() ) {
$tax_queries = array();
foreach ( $attributes as $attribute ) {
$term_id = $attribute['termId'];
$term = get_term( $term_id );
if ( $term && ! is_wp_error( $term ) ) {
$taxonomy = $term->taxonomy;
if ( ! array_key_exists( $taxonomy, $tax_queries ) ) {
$tax_queries[ $taxonomy ] = array(
'taxonomy' => $taxonomy,
'field' => 'term_id',
'terms' => array( $term_id ),
'operator' => 'IN',
);
} else {
$tax_queries[ $taxonomy ]['terms'][] = $term_id;
}
}
}
return array(
'tax_query' => array_values( $tax_queries ),
);
}
This is not necessarily the final solution; another alternative could be to ensure the information received via the $attributes
variable matches the expected by this method instead.
With this error out of the way, I was able to confirm the FE correctly displays all relevant products (in this test, all products with the colors blue and gray are listed, even though, in some cases, the images of the products displayed are matching the product variation options available and not necessarily the filtered colors (and this is pre-existing).
🗒️ Sidenote: do we have an issue opened for this already? It feels wrong to display a red hoodie if the filter is deliberately specifying only blue product variations should be displayed (I noticed this same behavior with the attribute filter before).
Checkbox behavior
Repeat steps 1–5 above.
Click on some terms checkboxes.
Notice that the parent checkbox will be in the indeterminate state ([-]).
✅ Confirmed the parent checkbox is in the indeterminate state:
Select all the children.
Notice parent checkbox will be checked.
✅ Confirmed the parent checkbox is checked when all children are selected:
Uncheck the parent checkbox.
All the children should be unchecked.
✅ Confirmed all children are unchecked, but with one limitation:
❌ When a combination of terms from different taxonomies is selected, if you uncheck all of them for one category in particular, the terms are removed for all categories instead (not just the selected one):
Screen.Recording.2023-03-03.at.18.36.33.mov
Check the parent checkbox.
All the children should be checked.
✅ Confirmed all children are checked when the parent checkbox is clicked.
Uncheck a few children.
Check the parent checkbox.
All children should be checked.
✅ Confirmed this works as expected.
Token input behavior
Repeat steps 1–4 above.
Start typing something within the input.
The checklist should be correctly filtered showing breadcrumbs.
✅ Confirmed the checklist is correctly filtered and its showing the breadcrumbs:
❌ When the name of a term is typed, the token is added as expected, but the typed text is still displayed on the side (you have to manually remove the text to be able to search for the next term):
Screen.Recording.2023-03-03.at.18.45.22.mov
Try the behavior for both collapsed and uncollapsed states.
✅ Confirmed it works as expected with both collapsed and uncollapsed states.
Every time a checkbox is checked, it should have the corresponding token in the input field and vice-versa.
✅ Confirmed it works as expected.
Tokens can be correctly removed from the input field and this will uncheck the corresponding checkbox.
✅ Confirmed it works as expected.
Hey @nefeline! Thanks a lot for the extremely helpful review!
Ouch, silly me for not noticing this. I did some last minute refactoring of the types and I didn't check this thing. Thank you for providing also an alternative solution, but as you mentioned, I fixed it upstream so that the function receives the data it expects.
Yea, this was pre-existing in all our implementations (including the front-end filter). We do not have an issue for this AFAIK. Also I'm not sure how the products work under the hood. If you go to the product page, you'll see that “red hoodie” is the default picture. Pictures are then assigned to product “variations”, not attributes themselves. So I guess this would need a deep dive into how all these things work to make it happen. Definitely weird, though, I would agree.
Fixed. My math skills faltered 😅
Unfortunately this is a known issue I mentioned in the OP:
There was a workaround I had found, but it had its own weird side-effects unfortunately (the text would be removed also when clicking on checkboxes in my workaround). Having to decide between the lesser evil, I chose this route, which, in the end, preserves the text typed by the user which can be useful when you are doing a partial search and not a full match. It's not ideal, but it was either this, my other workaround, or rebuilding the Token input from scratch. I'll try and contribute upstream for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the feedback! Regarding the code, I've left just one minor question & optional change, other than that this is looking sharp & I'm pre-approving!
Thanks a lot for the extremely helpful review!
Anytime! 🙌
I fixed it upstream so that the function receives the data it expects.
✅ Awesome, thanks! I can confirm the notice is now gone, and the correct products are displayed on the FE with this fix in place.
Fixed. My math skills faltered 😅
✅ 😄 It happens to the best of us, thanks for addressing the issue! A can confirm when a combination of terms from different taxonomies is selected; if you uncheck all of them for one category in particular, the correct/expected terms are now removed, and not all of them.
There was a workaround I had found, but it had its own weird side-effects unfortunately (the text would be removed also when clicking on checkboxes in my workaround). Having to decide between the lesser evil, I chose this route, which, in the end, preserves the text typed by the user which can be useful when you are doing a partial search and not a full match. It's not ideal, but it was either this, my other workaround, or rebuilding the Token input from scratch. I'll try and contribute upstream for this.
Gotcha: sorry I missed the description of this limitation in particular. The path chosen here makes sense, thanks for clarifying!
Yea, this was pre-existing in all our implementations (including the front-end filter). We do not have an issue for this AFAIK. Also I'm not sure how the products work under the hood. If you go to the product page, you'll see that “red hoodie” is the default picture. Pictures are then assigned to product “variations”, not attributes themselves. So I guess this would need a deep dive into how all these things work to make it happen. Definitely weird, though, I would agree.
Thanks 👍 ! I'm opening a separate issue so we can keep track & investigate later on when time permits.
This PR is meant to improve the UI and UX behind the Attributes filter within the Inspector Controls of the “Products (Beta)“ block. While doing so, I wanted to use the original Attribute selector component we had (
AttributeTermControl
) instead of creating a new, almost same, one. As such, this PR does a bunch of things to improve and extend that component as well.In particular:
useProductAttributes
hook that was created for the draft filter. The refactored hook is now available in the shared code, instead of just theproduct-query
block directory, and has been modified to achieve the following:partial objects compared to the API? Maybe a follow-up to this could be to check why the attributes stored in the settings are incomplete?).
SearchListControl
component with the following additions:CheckboxControl
instead of radio buttons for items having children (besides using the core component, it also helps becauseCheckboxControl
includes theindeterminate
state, which was required by the design spec).ProductAttributeTermControl
component and remove thewithAttributes
HOC wrapping it, in favor of theuseProductAttributes
hook.Closes #8135
Accessibility
prefers-reduced-motion
Other Checks
Screenshots
Testing
Automated Tests
User Facing Testing
Simple happy path
Checkbox behavior
[-]
).Token input behavior
Notes
Note that the
SearchListControl
is used in other components which may or may not be covered by E2E tests, and this PR refactors it. I tried my best to avoid any possible regression, but please do double check with components you know. Some examples are Filter By Attribute block or Featured Product/Category.Known issues
item.tsx#L118
due to wrong Gutenberg types. Will update them upstream.WooCommerce Visibility
Changelog